[lldb] Use llvm::Error instead of CommandReturnObject for error reporting #125125
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: Jonas Devlieghere (JDevlieghere) ChangesUse Full diff: https://github.com/llvm/llvm-project/pull/125125.diff 5 Files Affected:
diff --git a/lldb/include/lldb/Interpreter/Options.h b/lldb/include/lldb/Interpreter/Options.h
index 9a6a17c2793fa4..864bda6f24c8cc 100644
--- a/lldb/include/lldb/Interpreter/Options.h
+++ b/lldb/include/lldb/Interpreter/Options.h
@@ -76,12 +76,12 @@ class Options {
// This gets passed the short option as an integer...
void OptionSeen(int short_option);
- bool VerifyOptions(CommandReturnObject &result);
+ llvm::Error VerifyOptions();
// Verify that the options given are in the options table and can be used
// together, but there may be some required options that are missing (used to
// verify options that get folded into command aliases).
- bool VerifyPartialOptions(CommandReturnObject &result);
+ llvm::Error VerifyPartialOptions();
void OutputFormattedUsageText(Stream &strm,
const OptionDefinition &option_def,
diff --git a/lldb/source/Interpreter/CommandAlias.cpp b/lldb/source/Interpreter/CommandAlias.cpp
index c5971b52f837fa..b29b06c48c84f5 100644
--- a/lldb/source/Interpreter/CommandAlias.cpp
+++ b/lldb/source/Interpreter/CommandAlias.cpp
@@ -10,6 +10,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/Support/ErrorHandling.h"
+#include "llvm/Support/FormatAdapters.h"
#include "lldb/Interpreter/CommandInterpreter.h"
#include "lldb/Interpreter/CommandObject.h"
@@ -20,20 +21,17 @@
using namespace lldb;
using namespace lldb_private;
-static bool ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
- llvm::StringRef options_args,
- OptionArgVectorSP &option_arg_vector_sp) {
- bool success = true;
+static llvm::Error
+ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
+ llvm::StringRef options_args,
+ OptionArgVectorSP &option_arg_vector_sp) {
OptionArgVector *option_arg_vector = option_arg_vector_sp.get();
if (options_args.size() < 1)
- return true;
+ return llvm::Error::success();
Args args(options_args);
std::string options_string(options_args);
- // TODO: Find a way to propagate errors in this CommandReturnObject up the
- // stack.
- CommandReturnObject result(false);
// Check to see if the command being aliased can take any command options.
Options *options = cmd_obj_sp->GetOptions();
if (options) {
@@ -45,34 +43,30 @@ static bool ProcessAliasOptionsArgs(lldb::CommandObjectSP &cmd_obj_sp,
llvm::Expected<Args> args_or =
options->ParseAlias(args, option_arg_vector, options_string);
- if (!args_or) {
- result.AppendError(toString(args_or.takeError()));
- result.AppendError("Unable to create requested alias.\n");
- return false;
- }
+ if (!args_or)
+ return llvm::createStringError(
+ llvm::formatv("unable to create alias: {0}",
+ llvm::fmt_consume(args_or.takeError())));
args = std::move(*args_or);
- options->VerifyPartialOptions(result);
- if (!result.Succeeded() &&
- result.GetStatus() != lldb::eReturnStatusStarted) {
- result.AppendError("Unable to create requested alias.\n");
- return false;
- }
+ if (llvm::Error error = options->VerifyPartialOptions())
+ return error;
}
if (!options_string.empty()) {
- if (cmd_obj_sp->WantsRawCommandString())
- option_arg_vector->emplace_back(CommandInterpreter::g_argument,
- -1, options_string);
- else {
+ if (cmd_obj_sp->WantsRawCommandString()) {
+ option_arg_vector->emplace_back(CommandInterpreter::g_argument, -1,
+ options_string);
+ } else {
for (auto &entry : args.entries()) {
if (!entry.ref().empty())
- option_arg_vector->emplace_back(std::string(CommandInterpreter::g_argument), -1,
- std::string(entry.ref()));
+ option_arg_vector->emplace_back(
+ std::string(CommandInterpreter::g_argument), -1,
+ std::string(entry.ref()));
}
}
}
- return success;
+ return llvm::Error::success();
}
CommandAlias::CommandAlias(CommandInterpreter &interpreter,
@@ -85,10 +79,15 @@ CommandAlias::CommandAlias(CommandInterpreter &interpreter,
m_option_args_sp(new OptionArgVector),
m_is_dashdash_alias(eLazyBoolCalculate), m_did_set_help(false),
m_did_set_help_long(false) {
- if (ProcessAliasOptionsArgs(cmd_sp, options_args, m_option_args_sp)) {
+ if (llvm::Error error =
+ ProcessAliasOptionsArgs(cmd_sp, options_args, m_option_args_sp)) {
+ // FIXME: Find a way to percolate this error up.
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Host), std::move(error),
+ "ProcessAliasOptionsArgs failed: {0}");
+ } else {
m_underlying_command_sp = cmd_sp;
for (int i = 0;
- auto cmd_entry = m_underlying_command_sp->GetArgumentEntryAtIndex(i);
+ auto *cmd_entry = m_underlying_command_sp->GetArgumentEntryAtIndex(i);
i++) {
m_arguments.push_back(*cmd_entry);
}
@@ -159,8 +158,8 @@ void CommandAlias::GetAliasExpansion(StreamString &help_string) const {
help_string.Printf(" %s", value.c_str());
} else {
help_string.Printf(" %s", opt.c_str());
- if ((value != CommandInterpreter::g_no_argument)
- && (value != CommandInterpreter::g_need_argument)) {
+ if ((value != CommandInterpreter::g_no_argument) &&
+ (value != CommandInterpreter::g_need_argument)) {
help_string.Printf(" %s", value.c_str());
}
}
diff --git a/lldb/source/Interpreter/CommandObject.cpp b/lldb/source/Interpreter/CommandObject.cpp
index 559e2e7a76dd99..bd0a792b450ca7 100644
--- a/lldb/source/Interpreter/CommandObject.cpp
+++ b/lldb/source/Interpreter/CommandObject.cpp
@@ -124,7 +124,7 @@ bool CommandObject::ParseOptions(Args &args, CommandReturnObject &result) {
error = Status::FromError(args_or.takeError());
if (error.Success()) {
- if (options->VerifyOptions(result))
+ if (options->VerifyOptions())
return true;
} else {
result.SetError(error.takeError());
@@ -306,7 +306,7 @@ void CommandObject::HandleArgumentCompletion(
assert(entry_ptr && "We said there was one entry, but there wasn't.");
return; // Not worth crashing if asserts are off...
}
-
+
CommandArgumentEntry &entry = *entry_ptr;
// For now, we only handle the simple case of one homogenous argument type.
if (entry.size() != 1)
@@ -492,21 +492,21 @@ bool CommandObject::IsPairType(ArgumentRepetitionType arg_repeat_type) {
(arg_repeat_type == eArgRepeatPairRangeOptional);
}
-std::optional<ArgumentRepetitionType>
+std::optional<ArgumentRepetitionType>
CommandObject::ArgRepetitionFromString(llvm::StringRef string) {
return llvm::StringSwitch<ArgumentRepetitionType>(string)
- .Case("plain", eArgRepeatPlain)
- .Case("optional", eArgRepeatOptional)
- .Case("plus", eArgRepeatPlus)
- .Case("star", eArgRepeatStar)
- .Case("range", eArgRepeatRange)
- .Case("pair-plain", eArgRepeatPairPlain)
- .Case("pair-optional", eArgRepeatPairOptional)
- .Case("pair-plus", eArgRepeatPairPlus)
- .Case("pair-star", eArgRepeatPairStar)
- .Case("pair-range", eArgRepeatPairRange)
- .Case("pair-range-optional", eArgRepeatPairRangeOptional)
- .Default({});
+ .Case("plain", eArgRepeatPlain)
+ .Case("optional", eArgRepeatOptional)
+ .Case("plus", eArgRepeatPlus)
+ .Case("star", eArgRepeatStar)
+ .Case("range", eArgRepeatRange)
+ .Case("pair-plain", eArgRepeatPairPlain)
+ .Case("pair-optional", eArgRepeatPairOptional)
+ .Case("pair-plus", eArgRepeatPairPlus)
+ .Case("pair-star", eArgRepeatPairStar)
+ .Case("pair-range", eArgRepeatPairRange)
+ .Case("pair-range-optional", eArgRepeatPairRangeOptional)
+ .Default({});
}
static CommandObject::CommandArgumentEntry
diff --git a/lldb/source/Interpreter/Options.cpp b/lldb/source/Interpreter/Options.cpp
index 893a3b71604ba8..fdadba62987d36 100644
--- a/lldb/source/Interpreter/Options.cpp
+++ b/lldb/source/Interpreter/Options.cpp
@@ -138,46 +138,6 @@ void Options::OptionsSetUnion(const OptionSet &set_a, const OptionSet &set_b,
}
}
-bool Options::VerifyOptions(CommandReturnObject &result) {
- bool options_are_valid = false;
-
- int num_levels = GetRequiredOptions().size();
- if (num_levels) {
- for (int i = 0; i < num_levels && !options_are_valid; ++i) {
- // This is the correct set of options if: 1). m_seen_options contains
- // all of m_required_options[i] (i.e. all the required options at this
- // level are a subset of m_seen_options); AND 2). { m_seen_options -
- // m_required_options[i] is a subset of m_options_options[i] (i.e. all
- // the rest of m_seen_options are in the set of optional options at this
- // level.
-
- // Check to see if all of m_required_options[i] are a subset of
- // m_seen_options
- if (IsASubset(GetRequiredOptions()[i], m_seen_options)) {
- // Construct the set difference: remaining_options = {m_seen_options} -
- // {m_required_options[i]}
- OptionSet remaining_options;
- OptionsSetDiff(m_seen_options, GetRequiredOptions()[i],
- remaining_options);
- // Check to see if remaining_options is a subset of
- // m_optional_options[i]
- if (IsASubset(remaining_options, GetOptionalOptions()[i]))
- options_are_valid = true;
- }
- }
- } else {
- options_are_valid = true;
- }
-
- if (options_are_valid) {
- result.SetStatus(eReturnStatusSuccessFinishNoResult);
- } else {
- result.AppendError("invalid combination of options for the given command");
- }
-
- return options_are_valid;
-}
-
// This is called in the Options constructor, though we could call it lazily if
// that ends up being a performance problem.
@@ -590,13 +550,50 @@ void Options::GenerateOptionUsage(Stream &strm, CommandObject &cmd,
strm.SetIndentLevel(save_indent_level);
}
+llvm::Error Options::VerifyOptions() {
+ bool options_are_valid = false;
+
+ int num_levels = GetRequiredOptions().size();
+ if (num_levels) {
+ for (int i = 0; i < num_levels && !options_are_valid; ++i) {
+ // This is the correct set of options if: 1). m_seen_options contains
+ // all of m_required_options[i] (i.e. all the required options at this
+ // level are a subset of m_seen_options); AND 2). { m_seen_options -
+ // m_required_options[i] is a subset of m_options_options[i] (i.e. all
+ // the rest of m_seen_options are in the set of optional options at this
+ // level.
+
+ // Check to see if all of m_required_options[i] are a subset of
+ // m_seen_options
+ if (IsASubset(GetRequiredOptions()[i], m_seen_options)) {
+ // Construct the set difference: remaining_options = {m_seen_options} -
+ // {m_required_options[i]}
+ OptionSet remaining_options;
+ OptionsSetDiff(m_seen_options, GetRequiredOptions()[i],
+ remaining_options);
+ // Check to see if remaining_options is a subset of
+ // m_optional_options[i]
+ if (IsASubset(remaining_options, GetOptionalOptions()[i]))
+ options_are_valid = true;
+ }
+ }
+ } else {
+ options_are_valid = true;
+ }
+
+ if (!options_are_valid)
+ return llvm::createStringError(
+ "invalid combination of options for the given command");
+
+ return llvm::Error::success();
+}
+
// This function is called when we have been given a potentially incomplete set
// of options, such as when an alias has been defined (more options might be
// added at at the time the alias is invoked). We need to verify that the
// options in the set m_seen_options are all part of a set that may be used
// together, but m_seen_options may be missing some of the "required" options.
-
-bool Options::VerifyPartialOptions(CommandReturnObject &result) {
+llvm::Error Options::VerifyPartialOptions() {
bool options_are_valid = false;
int num_levels = GetRequiredOptions().size();
@@ -613,7 +610,11 @@ bool Options::VerifyPartialOptions(CommandReturnObject &result) {
}
}
- return options_are_valid;
+ if (!options_are_valid)
+ return llvm::createStringError(
+ "invalid combination of options for the given command");
+
+ return llvm::Error::success();
}
bool Options::HandleOptionCompletion(CompletionRequest &request,
diff --git a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
index 4ca8bd2f9085df..82f18c5fe37a21 100644
--- a/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
+++ b/lldb/source/Plugins/StructuredData/DarwinLog/StructuredDataDarwinLog.cpp
@@ -975,8 +975,6 @@ EnableOptionsSP ParseAutoEnableOptions(Status &error, Debugger &debugger) {
EnableOptionsSP options_sp(new EnableOptions());
options_sp->NotifyOptionParsingStarting(&exe_ctx);
- CommandReturnObject result(debugger.GetUseColor());
-
// Parse the arguments.
auto options_property_sp =
debugger.GetPropertyValue(nullptr,
@@ -1013,8 +1011,13 @@ EnableOptionsSP ParseAutoEnableOptions(Status &error, Debugger &debugger) {
return EnableOptionsSP();
}
- if (!options_sp->VerifyOptions(result))
+ if (llvm::Error error = options_sp->VerifyOptions()) {
+ LLDB_LOG_ERROR(
+ log, std::move(error),
+ "Parsing plugin.structured-data.darwin-log.auto-enable-options value "
+ "failed: {0}");
return EnableOptionsSP();
+ }
// We successfully parsed and validated the options.
return options_sp;
|
…ting Use `llvm::Error` instead of `CommandReturnObject` for error reporting. The command return objects were populated with errors but never displayed. With this patch they're at least logged.
JDevlieghere
force-pushed
the
use-error-instead-of-commandreturnobject
branch
from
218db81 to
24ddc55
Compare
JDevlieghere
commented
Jan 30, 2025
Comment on lines
+553
to
+589
| llvm::Error Options::VerifyOptions() { | ||
| bool options_are_valid = false; | ||
|
|
||
| int num_levels = GetRequiredOptions().size(); | ||
| if (num_levels) { | ||
| for (int i = 0; i < num_levels && !options_are_valid; ++i) { | ||
| // This is the correct set of options if: 1). m_seen_options contains | ||
| // all of m_required_options[i] (i.e. all the required options at this | ||
| // level are a subset of m_seen_options); AND 2). { m_seen_options - | ||
| // m_required_options[i] is a subset of m_options_options[i] (i.e. all | ||
| // the rest of m_seen_options are in the set of optional options at this | ||
| // level. | ||
|
|
||
| // Check to see if all of m_required_options[i] are a subset of | ||
| // m_seen_options | ||
| if (IsASubset(GetRequiredOptions()[i], m_seen_options)) { | ||
| // Construct the set difference: remaining_options = {m_seen_options} - | ||
| // {m_required_options[i]} | ||
| OptionSet remaining_options; | ||
| OptionsSetDiff(m_seen_options, GetRequiredOptions()[i], | ||
| remaining_options); | ||
| // Check to see if remaining_options is a subset of | ||
| // m_optional_options[i] | ||
| if (IsASubset(remaining_options, GetOptionalOptions()[i])) | ||
| options_are_valid = true; | ||
| } | ||
| } | ||
| } else { | ||
| options_are_valid = true; | ||
| } | ||
|
|
||
| if (!options_are_valid) | ||
| return llvm::createStringError( | ||
| "invalid combination of options for the given command"); | ||
|
|
||
| return llvm::Error::success(); | ||
| } |
There was a problem hiding this comment.
I moved this so it's closer to VerifyPartialOptions
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
labath
approved these changes
Jan 31, 2025
JDevlieghere
added a commit
to JDevlieghere/llvm-project
that referenced
this pull request
Jan 31, 2025
Name and line number are part of different option groups and are not compatible. (lldb) breakpoint set -n foo -l 10 error: invalid combination of options for the given command The help output for `breakpoint set` confirms this. This patch updates the test to use two compatible options. With the improved error reporting from llvm#125125 this becomes an issue.
JDevlieghere
added a commit
that referenced
this pull request
Jan 31, 2025
…25270) Name and line number are part of different option groups and are not compatible. ``` (lldb) breakpoint set -n foo -l 10 error: invalid combination of options for the given command ``` The help output for `breakpoint set` confirms this. This patch updates the test to use two compatible options. With the improved error reporting from #125125 this becomes an issue.
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 5, 2025
…vm#125270) Name and line number are part of different option groups and are not compatible. ``` (lldb) breakpoint set -n foo -l 10 error: invalid combination of options for the given command ``` The help output for `breakpoint set` confirms this. This patch updates the test to use two compatible options. With the improved error reporting from llvm#125125 this becomes an issue. (cherry picked from commit dbabad0)
JDevlieghere
added a commit
to swiftlang/llvm-project
that referenced
this pull request
Feb 5, 2025
…ting (llvm#125125) Use `llvm::Error` instead of `CommandReturnObject` for error reporting. The command return objects were populated with errors but never displayed. With this patch they're at least logged. (cherry picked from commit 6deee0d)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use
llvm::Errorinstead ofCommandReturnObjectfor error reporting. The command return objects were populated with errors but never displayed. With this patch they're at least logged.